Updating provider config #3468
Updating provider config #3468
Conversation
|
@kd @peppescg would you mind adding your opinion on the API? @evankanderson you have the most experience with protobuf, could you check the patches to the proto file and the mergo usage? Is there a nicer way? Thank you all for taking a look. |
| Context context = 1; | ||
| ProviderPatch patch = 2; | ||
|
|
||
| google.protobuf.FieldMask update_mask = 3; |
There was a problem hiding this comment.
I don't recall exactly how good the library support is for update_mask. It's real handy as a client, but make sure it won't get you into too much trouble in the implementation.
There was a problem hiding this comment.
I actually don't use it now in the implementation. We might want to reconsider later, but yeah, it seems like a lot of trouble for very questionable gain (Unless the FE devs disgree, going to ping them now for API review)
| }, | ||
| "parameters": [ | ||
| { | ||
| "name": "patch", |
There was a problem hiding this comment.
I'm assuming this name doesn't show up anywhere.
There was a problem hiding this comment.
It does not, e.g. one calls the endpoint as:
curl -XPATCH -H "Authorization: Bearer $MINDER_BEARER_TOKEN" 'http://localhost:8080/api/v1/providers?context.project=bcf5bc68-aa81-4f59-96ae-7dceb5c3663f&context.provider=github-app-jakubtestorg' -d '{ "config": { "auto_registration": { "enabled": ["repository"] }} }'
in other words, the body already contains the provider attributes.
| return nil, status.Errorf(codes.InvalidArgument, "provider name is required") | ||
| } | ||
|
|
||
| err := s.providerManager.PatchProviderConfig(ctx, providerName, projectID, req.GetPatch().GetConfig().AsMap()) |
There was a problem hiding this comment.
Should this be getting a provider instance and then calling PatchConfig on the provider instance? I'm okay with having this on providerManager, I just don't have a feel for when we create an instance and when we don't.
There was a problem hiding this comment.
yeah, me neither. Getting a provider instance and then patching it is pretty much what the implementation of PatchProviderConfig does now.
@dmjb do you have an opinion since you created the ProviderManager interface?
jhrozek
force-pushed
the
update_prov_cfg
branch
3 times, most recently
from
1ba11d6
to
614f0d7
Compare
jhrozek
force-pushed
the
update_prov_cfg
branch
2 times, most recently
from
8438157
to
3dde870
Compare
@evankanderson I think yes at this point. There's an alternative approach in PR #3588 that's pure-proto based, but after discussions with @blkt and @dmjb we lean towards going with this simple approach for now and then @blkt would instead try to extend the code with |
This merely adds the PatchProvider rpc changes to keep the other patches in the same PR minimal and don't mix the RPC changes with the logic changes.
…dating config The providerStore.Update method only exposes the new config as a parameter to change. Others might be added as we have a use-case for them - this is not public API.
Building atop the providerStore.Update method, we add a PatchProvider handler that so far only allows updating the config.
Since storing the whole config in the database is problematic for a number of reasons, like hard patching or worse ability to change defaults, let's change the way we treat the provider configuration in the database. Instead of storing the full config, we treat the provider config as an override. To that end, we store a structure with defaults in the code and make all the fields in the protobuf messages optional which causes the resulting Go structures to have pointers to fields instead of direct attributes. We store the config override verbatim, after just having unmarshalled and marshalled back to get rid of any extra keys. Then we merge with the default config on retrieving the provider configuration from the database.
Summary
Protobuf changes for providers update - just adds the protobufs, nothing else
Extend the ProviderStore with an Update method, so far only allows updating config - we used to have a database-level Update, but not exposed through the provider store interface
Add a PatchProvider server handler, currently just updating the config - this is the main part of the PR. Adds a new method PatchProviderConfig to the
ProviderServiceinterface.This method uses the mergo library to merge the provided config with the currently stored config and then update the provider record in the DB.
Treat the provider config as an override, keep the defaults in the code - Since storing the whole config in the database is problematic for a number of reasons, like hard patching or worse ability to change defaults, let's change the way we treat the provider configuration in the database. Instead of storing the full config, we treat the provider config as an override. To that end, we store a structure with defaults in the code and make all the fields in the protobuf messages optional which causes the resulting Go structures to have pointers to fields instead of direct attributes.
We store the config override verbatim, after just having unmarshalled and marshalled back to get rid of any extra keys. Then we merge with the default config on retrieving the provider configuration from the database.
Fixes: #3265
Fixes: #3511
Change Type
Mark the type of change your PR introduces:
Testing
Review Checklist: